Claude finished @Hmbown's task in 1m 44s —— View job

Code Review

• Gather context and read changed files
• Review crates/tui/src/tools/subagent/mod.rs
• Review crates/tui/src/core/engine/tests.rs
• Post findings

Summary

Clean, well-scoped change. The _for(input) override pattern is the right hook — it keeps ToolSpec callers in control and doesn't require touching the batch planner itself. Error handling is correct: all parse_agent_tool_action failures fall through to safe, conservative defaults (false / Required). The two new tests exercise the exact batch-planner code path being fixed.

Three things worth addressing:

Finding 1 — Misleading doc comment on is_read_only_for (minor / docs)

File: crates/tui/src/tools/subagent/mod.rs:3563

/// #3801: status/peek/cancel actions are read-only queries of manager state.
fn is_read_only_for(&self, input: &Value) -> bool {
    matches!(
        parse_agent_tool_action(input),
        Ok(AgentToolAction::Status) | Ok(AgentToolAction::Peek)
    )
}

The comment says cancel is read-only, but Cancel is not in the match arm and is definitely not read-only — it cancels a running agent (mutating manager state). The implementation is correct; the comment needs the word "cancel" removed.

Fix this →

Finding 2 — Missing negative test for non-YOLO mode (test coverage gap)

The PR's own security note says: "In non-YOLO modes, each agent launch still prompts for approval and is thus serial (expected). The parallel path only fires when auto_approve = true."

The two new tests only cover the auto-approved path (all plans have approval_required = false). There's no test asserting the inverse: that a detached-start agent plan with approval_required = true does not join a parallel batch. This is the most important guardrail to verify with a test, since tool_plan_can_join_parallel_batch checks !plan.approval_required and a regression there would silently re-serialize the fix.

Suggested test (mirrors the existing pattern):

#[test]
fn agent_start_requires_approval_does_not_join_parallel_batch() {
    let mut agent_start = make_plan_at(0, false, false, false, true); // approval_required=true
    agent_start.name = "agent".to_string();
    agent_start.detached_start = true;

    let batches = plan_tool_execution_batches(vec![agent_start]);
    assert_eq!(batches.len(), 1);
    assert!(matches!(&batches[0], ToolExecutionBatch::Serial(_)),
        "agent start requiring approval must not join parallel batch");
}

Fix this →

Finding 3 — No coverage for action=cancel parallelism isolation (minor)

The PR guarantees cancel stays serial (it returns Required and is not detached). There's no test asserting this. Given the security framing of this PR, a regression test guarding that action=cancel never joins a parallel batch would make the invariant machine-checkable. Low priority, but fits the pattern of the other negative tests in this file.

What's good

• starts_detached_for correctly returns true only for Start, not for Cancel. The Cancel action's approval_required=true would block it from a parallel batch anyway, but it's good to be conservative.
• approval_requirement_for falls to Required for any parse error or unknown action — no accidental auto-approval.
• Tests use make_plan_at directly rather than instantiating a real AgentTool, keeping them fast and focused on the batch planner, which is the correct seam to test here.
• The detached_start path in tool_plan_can_join_parallel_batch is unchanged — this PR's behaviour is purely additive, driven by the new ToolSpec impl.

Claude finished @Hmbown's task in 2m 51s —— View job

Code Review

• Load tools
• Read changed files and diff
• Analyze correctness, security, and test coverage
• Post findings

Summary

The batch-planner hook design is clean and the _for(input) override pattern is exactly right — input-aware routing without touching the planner itself. The three existing findings from the prior review pass remain unaddressed; one new correctness concern emerges from reading inspect_agent_from_input.

Finding 1 — is_read_only_for contract is wrong: Status/Peek mutate manager state (correctness)

File: crates/tui/src/tools/subagent/mod.rs:3561

is_read_only_for returns true for Status and Peek, but both code paths in inspect_agent_from_input unconditionally call:

let mut manager = manager.write().await;
manager.cleanup(COMPLETED_AGENT_RETENTION);

cleanup() takes a mutable reference and removes/archives completed-agent records — this is a real write to shared state. Consequences:

• Two "parallel" status calls in the same batch will contend on the write lock and serialize anyway, so the parallel-batch label buys nothing for them.
• A status call in the same batch as an agent start means cleanup fires concurrently with spawn (whichever gets the write lock first). Today this is safe because cleanup only removes COMPLETED_AGENT_RETENTION-old agents. But the is_read_only contract implies this path can never race with a write — a future caller relying on that assumption would be wrong.

Either inspect_agent_from_input should use manager.read() for the inspection portion and decouple cleanup into a separate path (making the is_read_only claim true), or is_read_only_for for Status/Peek should return false (and these actions enter the batch via detached_start or a new non-read-only parallel path instead).

Fix this →

Finding 2 — Misleading doc comment on is_read_only_for (docs, prior review Finding 1)

File: crates/tui/src/tools/subagent/mod.rs:3560

/// #3801: status/peek/cancel actions are read-only queries of manager state.
fn is_read_only_for(&self, input: &Value) -> bool {

cancel appears in the comment but not in the match arm — Cancel mutates manager state and is correctly excluded from the match. Remove "cancel" from the comment.

Fix this →

Finding 3 — No negative test: approval_required=true must not join parallel batch (test coverage, prior review Finding 2)

This is the most important invariant to machine-check: the security note in the PR says "the parallel path only fires when auto_approve = true." There's no test asserting that detached_start=true, approval_required=true produces a Serial batch.

#[test]
fn agent_start_requires_approval_does_not_join_parallel_batch() {
    let mut plan = make_plan_at(0, false, false, true, false); // approval_required=true
    plan.name = "agent".to_string();
    plan.detached_start = true;

    let batches = plan_tool_execution_batches(vec![plan]);
    assert_eq!(batches.len(), 1);
    assert!(
        matches!(&batches[0], ToolExecutionBatch::Serial(_)),
        "agent start requiring approval must not join parallel batch"
    );
}

A regression in tool_plan_can_join_parallel_batch's !plan.approval_required guard would be silently invisible without this test.

Fix this →

Finding 4 — No negative test: cancel must not join parallel batch (test coverage, prior review Finding 3)

No test guards the invariant that action=cancel is never batched in parallel. Since cancel returns Required approval and isn't detached, it can't currently join a parallel batch — but a test makes this machine-checkable and prevents silent regression.

What's good

• Error fallthrough in all four overrides is conservative (false / Required) — no accidental auto-approval or parallel-marking on parse failure.
• starts_detached_for correctly returns true only for Start, not Cancel. Cancel is also blocked by approval_required=true even if detached_start were accidentally set.
• The two new positive tests cover the exact dispatch-planner code path and use make_plan_at directly — fast and focused on the right seam.
• No changes to the batch planner itself; this is purely additive.